-
Notifications
You must be signed in to change notification settings - Fork 938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[naga wgsl-in] Attempt automatic conversion for arguments to user defined function calls #6577
base: trunk
Are you sure you want to change the base?
Conversation
There may be a discussion to be had regarding how to handle conversion errors, which can either be addressed here or in a follow up. Note the change made to the
However, the test only ensures that parsing succeeds, not validation. With this patch we now report an error during parsing (hence why we had to fix the shader code for the test to still pass):
Arguably the new error is more helpful as tells you the required type (though not printing the pointer's address space is unhelpful). Though of course the validator's error message could be improved. Additionally the "automatic conversions cannot convert..." line is a bit misleading, as we shouldn't really be attempting to automatically convert a ptr. FWIW I did try making us not attempt automatic conversions for non-abstract types, but that caused this test assertion to fail. Again because it expects an error during parsing, but by not attempting to convert non-abstract types that shader parses successfully (but still fails validation) |
For comparison, tint's error messages seem a bit more intuitive. With the pointer example from above: fn foo(a: ptr<private, f32>) -> f32 { return *a; }
fn bar() {
var x: f32 = 1.0;
let px = &x;
let py = foo(px);
} Tint outputs:
But in a case where an abstract type cannot convert correctly: fn foo(a: u32) {}
fn bar() {
foo(0.0);
}
Note the distinction between a completely mismatched type, and an abstract type that cannot be converted correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but I'm rather inexperienced with this part of Naga. Will give somebody from @gfx-rs/naga a (timeboxed) chance to look this over; holler if you'd like that, please!
.enumerate() | ||
.map(|(i, &arg)| { | ||
// Try to convert abstract values to the known argument types | ||
match ctx.module.functions[function] | ||
.arguments | ||
.get(i) | ||
.map(|arg| arg.ty) | ||
{ | ||
Some(arg_ty) => { | ||
let expr = self.expression_for_abstract(arg, ctx)?; | ||
ctx.try_automatic_conversions( | ||
expr, | ||
&crate::proc::TypeResolution::Handle(arg_ty), | ||
ctx.ast_expressions.get_span(arg), | ||
) | ||
} | ||
// Wrong number of arguments... just concretize the type here | ||
// and let the validator report the error. | ||
None => self.expression(arg, ctx), | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: Ah, one thing: gotta CHANGELOG
entry? 🙂 This sounds like a nice item to highlight there!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 🙂
…ined function calls When lowering arguments for a user-defined function call, avoid concretizing the argument types. Instead make use of the existing `try_automatic_conversions()` machinery to attempt to convert each argument to the type expected by the function. This is straightforward as user-defined functions only have a single overload. This additionally changes an argument type in the test parse_pointers() from `ptr<private>` to `ptr<function>`. The former is invalid code which is indeed caught by the validator, but the test only asserts that parsing succeeds, not validation. With this patch, this error is now caught during parsing which caused the test to fail.
c694931
to
c911aa7
Compare
@@ -100,6 +100,7 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148] | |||
- Fix textureNumLevels in the GLSL backend. By @magcius in [#6483](https://github.com/gfx-rs/wgpu/pull/6483). | |||
- Implement `quantizeToF16()` for WGSL frontend, and WGSL, SPIR-V, HLSL, MSL, and GLSL backends. By @jamienicol in [#6519](https://github.com/gfx-rs/wgpu/pull/6519). | |||
- Add support for GLSL `usampler*` and `isampler*`. By @DavidPeicho in [#6513](https://github.com/gfx-rs/wgpu/pull/6513). | |||
- Implement type inference for abstract arguments to user-defined functions. By @jamienicol in [#6577](https://github.com/gfx-rs/wgpu/pull/6577). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Doesn't this apply automatic type conversion, not just type inference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might not understand the distinction in terminology correctly. It will convert from AbstractInt
or AbstractFloat
to concrete types. It won’t convert from an already concrete type. I guess “conversion” is still a better word as “inference” relates to variable declaration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the term of art of "feasible automatic conversion", according to the WGSL spec.: https://www.w3.org/TR/WGSL/#feasible-automatic-conversion.
So, I'd say "type conversion" here would be uncontroversial, while "inference" probably refers to the wrong thing (i.e., figuring out what the type of a binding is when it's not specified given a RHS on assignment).
This looks ok to me but @jimblandy might want to take a look. |
Connections
Fixes #5523
Description
When lowering arguments for a user-defined function call, avoid concretizing the argument types. Instead make use of the existing
try_automatic_conversions()
machinery to attempt to convert each argument to the type expected by the function. This is straightforward as user-defined functions only have a single overload.This additionally changes an argument type in the test parse_pointers() from
ptr<private>
toptr<function>
. The former is invalid code which is indeed caught by the validator, but the test only asserts that parsing succeeds, not validation. With this patch, this error is now caught during parsing which caused the test to fail.Testing
Test suite passes. Added new tests.
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.